-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add Menu module #609
base: master
Are you sure you want to change the base?
Conversation
@@ -130,6 +133,10 @@ nix = { version = "0.27.1", optional = true, features = ["event"] } | |||
# clock | |||
chrono = { version = "0.4.38", optional = true, default_features = false, features = ["clock", "unstable-locales"] } | |||
|
|||
# menu | |||
freedesktop_entry_parser = { version = "1.3.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
freedesktop_entry_parser is needed to parse .desktop entries which it doesn't seem ironbar was doing until now. There's one other rust library that can do that but this one seemed lighter-weight and more stable.
unicode-segmentation is needed for truncating menu entries which is really needed because some common .desktop files (like the JDK) have really long names. As far as I can tell there's no actually safe way to truncate unicode strings in the Rust stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is .desktop file parsing happening in the desktop_file
module already. Did you not spot that, or does that not do what is necessary here?
In some other modules (music
, focused
) these are already using a (partially) common truncate
option that hooks into GTK's in-built truncation/elipsize system. I'd prefer to use that here, although I'm open to integrating unicode-segmentation
and having a words
mode there.
<summary>TOML</summary> | ||
|
||
```toml | ||
[[start.menu]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually sure this is correct, the way ironbar handles TOML is a bit confusing to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ironbar is just using the standard toml
crate, it's TOML that's confusing - there's two syntaxes for objects, and two for arrays.
Generally I write the corn config and then use its CLI to generate the other three formats, which isn't necessarily the most readable but at least ensures they're valid.
I think here, you actually want something like (shortened and untested):
[[start]]
type = "menu"
[[start.start]]
type = "custom"
} | ||
|
||
fn default_menu() -> Vec<MenuConfig> { | ||
vec![ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default menu that xfce has except that I picked the icons myself since I couldn't figure out how xfce does that (I think it might have a series of fallbacks due to lack of standardization in icon themes).
I looked through a number of icon themes and tried to identify icons that were available in the majority of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll road test this myself just to double-check before merging, but I'm sure that'll be fine. I need to rework some of the icon stuff soon anyway.
categories: [Foo, Bar] | ||
*/ | ||
|
||
fn parse_config( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My code organization is pretty sub-par here, I've been struggling with the fact that refactoring into functions is not free in Rust since it introduces a borrow. I thought about just writing a bunch of macros but I don't know if that's idiomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do sometimes stick a macro in a function to deal with repeat work, although I'm not a tremendous fan. My biggest issue is that they result in a performance hit to the language server when editing (I do need to refactor some of the existing ones out into inline functions really).
An alternative is to use closures, although I'm not sure if they incur any runtime performance penalty.
You can also actually use .iter().map().collect()
to collect into a HashMap (or IndexMap etc) directly, so long as you try to collect a tuple of (K, V)
.
); | ||
} | ||
MenuConfig::Custom(entry) => { | ||
let _ = entries.insert_sorted( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data flow is a little clunky because I was struggling to write code that avoided cloning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might be able to refactor to use into_iter
instead of iter
, which will give you the owned object instead of a reference. That'll allow you to move it.
That said, I don't think this is the end of the world if you do need to clone a few strings here as it's only init code.
let (mut center_entries, sections_by_cat) = parse_config(self.center, sections_by_cat); | ||
let (mut end_entries, sections_by_cat) = parse_config(self.end, sections_by_cat); | ||
|
||
let container2 = container.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For GTK objects I've just been spamming .clone()
until the borrow-checker stops complaining which seems to be the approach used elsewhere in ironbar. My understanding is the these are just pointers into objects managed by GTK so this doesn't really cost anything but then I don't understand why they don't implement Copy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a weird internal quirk of gtk-rs, they've got some pretty nuts macros going on, but it actually is Copy
sort of.
Each widget is basically a struct made of two pointers. The struct does not implement Copy
but each of the pointers do. This means the Clone
impl in reality does just create a new struct and Copy
the two fields.
As to why they don't implement it on the exposed structs, I've no idea. I guess it's more explicit?
let (w, h) = win.size(); | ||
let (x, y) = ev.position(); | ||
|
||
let hide = match pos { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all copied from popup.rs
but I didn't see any way to hook into that existing leave notify event. I could do some refactoring here but at least in my initial PR I was trying to keep changes isolated to the new module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, that's an annoying problem. You might be able to listen to the unmap event to get round that, which (in theory) will run when the window is hidden.
Thanks for your hard work on this, and for spending the time to go through and review already. I'll try and go through your comments in the next few days, and then my own full review off the back of that. Progress may be a little slower than usual on this from my end as I'm fairly busy at the moment, just FYI. |
Thanks for the heads up, Jake. |
@@ -0,0 +1,133 @@ | |||
Application menu that shows installed programs and optionally custom entries. Clicking the menu button will open the main menu, clicking on any application category will open a sub-menu with any installed applications that match. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder for myself to add an image
(no need to action anything)
| | Type | Default | Description | | ||
|--------------|------------|---------|-----------------------------------------------------------------------------------------------------| | ||
| `start` | `MenuEntry[]` | `[]` | List of menu entries | | ||
| `center` | `MenuEntry[]` | default XDG menu | List of menu entries. By default this shows a number of XDG entries that should cover all common applications | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set this, does it append to or replace the XDG entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will replace the default entries. It might be worthwhile to list the default entries in the documentation in case you want some of them but not others.
Co-authored-by: Jake Stanger <mail@jstanger.dev>
Co-authored-by: Jake Stanger <mail@jstanger.dev>
Co-authored-by: Jake Stanger <mail@jstanger.dev>
One thing I forget to mention earlier: whenever I cause a reflow (by hiding or showing a submenu) all the UI elements jump a little bit. I wasn't able to figure out why that is. |
That may be due to this: Line 135 in b5755ef
The popup will listen to changes in its size, and re-position itself to ensure it remains anchored to the widget it opened from. If appropriate, you could potentially avoid that by using the |
Oh cool, I spent a long time searching GTK issues and docs, didn't consider it could be something in Ironbar. I'll try to make that change and see if it fixes the issue. |
It doesn't seem like |
Okay that's annoying. I'll try and test myself when I can, and see if I can spot what's going on. |
This PR adds a new Menu widget which allows users to create XDG or custom menus that open after clicking on a button.
Resolves #534